Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancements #124

Merged
merged 10 commits into from
Feb 12, 2024
Merged

Enhancements #124

merged 10 commits into from
Feb 12, 2024

Conversation

psorensen
Copy link
Contributor

@psorensen psorensen commented Feb 6, 2024

Description of the Change

Kapture 2024-02-07 at 18 13 00

Closes #119

How to test the Change

  1. Add new winamp block
  2. Select a media file
  3. Confirm preview is shown
  4. Confirm preview toggle is located in the block's inspector control and works properly
  5. Confirm "Manage Media" block control is present when in preview mode
  6. Confirm "Show Preview" block control is present when in manage media mode
  7. Confirm preview setting persists after page update & reload
  8. Cionfr

Changelog Entry

Changed - Winamp skin preview setting to be on by default.
Changed - Moved preview toggle to inspector controls from block controls.
Changed - Moved webamp component to be nested inside the block, so that clicking that interacting the component also activates the block selection

Credits

Props @psorensen

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

since we're setting the default value of preview to true, we can't return return the placeholder image early since it prevents the edit controls from rendering
@psorensen psorensen marked this pull request as draft February 6, 2024 23:01
@jeffpaul jeffpaul added this to the 1.4.0 milestone Feb 6, 2024
@psorensen
Copy link
Contributor Author

@dkotter @jeffpaul I'm stuck making the tests work after this change - by setting the preview to default to true, the winamp canvas element is blocking cypress from clicking the publish button. The error message suggests forcing the click, but the click in question is handled upstream by the cypress utils. Additionally, this could be hinting at an accessibility issue that might make us reconsider showing the preview by default. Any thoughts here?

  1) Admin can publish posts with winamp block
       Can insert the block and publish the post:
     CypressError: Timed out retrying after 4050ms: `cy.click()` failed because this element:

`<button type="button" aria-disabled="false" class="components-button editor-post-publish-button editor-post-publish-button__button is-primary">Publish</button>`

is being covered by another element:

`<canvas height="343" width="431" style="height: 100%; width: 100%; display: block;"></canvas>`

Fix this problem, or use {force: true} to disable error checking.

https://on.cypress.io/element-cannot-be-interacted-with

@dkotter
Copy link
Collaborator

dkotter commented Feb 7, 2024

A couple concerns I have:

  1. The issue with the preview, which I'm assuming is what's causing the test failures, is it renders in a fixed position, so as you scroll in the editor, the preview moves with you and covers everything else. This was a problem I was never able to solve in the initial build which is one reason why we don't show the preview by default. It doesn't work this way on the front-end so I'm assuming something about the Block Editor (maybe some other conflicting styles) causes it to function this way.
  2. You can't add new media files in the preview mode. Just my personal opinion here but by rendering the preview by default anytime there's audio files, you have to switch that off to add additional files. And with that option now in the sidebar, it makes it harder to do that

I think we need to solve the fixed positioning issues if we want to have the preview on by default. Otherwise it's pretty annoying, especially if you have additional content in the post besides just this block, as it ends up covering everything.

@psorensen
Copy link
Contributor Author

@dkotter I took a pass at making what I feel is a pretty good UX solution here. The approach is a little heavy handed, but the webamp library is pretty limited in terms of what we're trying to accomplish. The updates now move the webamp component inside the block markup and positions it neatly, which a.) activates the block when the user interacts with the winamp component and b.) looks a lot nicer than the previous position which was just randomly floating on the page. Check out the gif in the PR description and let me know what you think. cc @jeffpaul

@psorensen psorensen marked this pull request as ready for review February 8, 2024 02:24
src/webamp.js Outdated Show resolved Hide resolved
@dkotter
Copy link
Collaborator

dkotter commented Feb 12, 2024

This looks good to me! Nice work here @psorensen

@dkotter dkotter merged commit 5bc4622 into develop Feb 12, 2024
8 of 9 checks passed
@dkotter dkotter deleted the enhancements branch February 12, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default block considerations
3 participants